Communities: Show relevant tokens #18636
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilmotta
commented
Jan 26, 2024
| @@ -24,7 +24,7 @@ | |||
| [:address [:maybe :string]] | |||
| [:emoji [:maybe :string]] | |||
| [:customization-color [:maybe [:or :string :keyword]]]]] | |||
| [:token-details {:optional true} [:maybe [:vector required-tokens/?schema]]] | |||
There was a problem hiding this comment.
Note: :vector is rarely the schema we want to use because it will fail when lazy sequences are passed.
ilmotta
commented
Jan 26, 2024
| [utils.re-frame :as rf])) | ||
|
|
||
| (defn get-permissioned-balances | ||
| [{[event-id _] :original-event} [community-id]] |
There was a problem hiding this comment.
Note: this is a nice way to grab the current event ID. Often the case when we want to log a descriptive error.
Jenkins BuildsClick to see older builds (27)
|
ilmotta
commented
Jan 26, 2024
| [:decimals int?] | ||
| [:amount string?]]]]) | ||
|
|
||
| (schema/=> get-permissioned-balances-success |
There was a problem hiding this comment.
@yqrashawn There's a strong pattern in the way we write schemas for event handlers. We could go just a small step further to help alleviate the verbosity for 95% of our use cases. Something to be discussed, but sharing here to give some visibility of the idea.
In the example below, the first argument to :=> would be the event args directly, the rest is almost always the same, and when they aren't, the dev would be able to use schema/=> instead.
(schema/event=> get-permissioned-balances-success
[:=>
[:catn
[:community-id string?]
[:response ?permissioned-balances-response]]
[:map {:closed true}
[:db map?]]])cc @clauxx
There was a problem hiding this comment.
Would be great to get rid of the boilerplate, I agree. Not as verbose as events, but for quo components we could do the same and just pass a prop map schema.
There was a problem hiding this comment.
Yes, we definitely need this.
I'm thinking about something like ring's middleware and wrap- functions
Or functions start with assoc- or walk- to transform schema.
These kinds of functions are easier to document, understand and maintain.
I checked Malli's schema transformation the other day. There are these get assoc functions implemented for [LensSchema](https://github.com/metosin/malli/blob/1d6efc23fef01ab333f872b357760de61eac0a21/src/malli/util.cljc#L303), but I didn't manage to figure out what's lens schema. Maybe something like specter?
If that's what we are using. It would be great.
There was a problem hiding this comment.
I'm thinking about something like ring's middleware and wrap- functions
@yqrashawn Last week I wrote a malli transformer similar to malli.util/optional-keys just to get the feeling of it and to solve one pain point I have in status-mobile. No PR, just experimentation. Transformers are interesting also because they're just functions and we can compose them, which if I understood you correctly is something you would like to have.
OTOH, transformers may be a bazooka to solve a simple problem of just eliminating the most essential boilerplate to define event handler schemas. For that, we just need a vanilla Clojure function and no need to use specific Malli APIs.
but I didn't manage to figure out what's lens schema.
I only know what lenses are and why they exist in functional languages, but I have zero knowledge about this part of Malli. Looks like something to be explored!
Maybe something like specter?
I've never used specter outside playground stuff, but it's surely one of a kind Clojure library. A bit too alien if you ask me.
There was a problem hiding this comment.
OTOH, transformers may be a bazooka to solve a simple problem of just eliminating the most essential boilerplate to define event handler schemas. For that, we just need a vanilla Clojure function and no need to use specific Malli APIs.
Totally agree
clauxx
reviewed
Jan 26, 2024
Comment on lines
25
to
27
| [:fx [:vector {:min 1 :max 1} :schema.common/rpc-call]]]]) | ||
| [:fx | ||
| [:tuple | ||
| [:tuple [:= :json-rpc/call] :schema.common/rpc-call]]]]]) |
There was a problem hiding this comment.
Should we be rigid about the returned effects map schema? From instrumenting events, I see most value from the arguments schema and by instrumenting the args from :json-rpc/call (or any other event/effect), we wouldn't need to check the args everywhere it's used. WDYT?
There was a problem hiding this comment.
Good point. There's a balance to strike. We have some events checking only that :fx is a map. Here I wanted to make sure it's nothing more than a tuple of tuples to experiment a little bit. I don't think this should be done all the time.
But I think the more we spec events, the more we should think about coming up with a schema representing re-frame's effects map. Our guidelines recommend using only :db and :fx in the returned map, so we could reinforce this idea more generally.
This generality won't be able to check something like I did OTOH, it will be more "open", but I think that's okay.
There was a problem hiding this comment.
Agreed, enforcing the guideline through the schema here would be more useful than the specific events and effects that are returned 👍. Can we instrument the :json-rpc/call effect instead?
There was a problem hiding this comment.
Oh definitely, we should instrument all highly used utilities/functions IMO. Those are the ones where it pays off to have schemas.
We had an agreement in the beginning to not spec non-component stuff, but time has passed and the team seems to be okay with Malli and a few are even enjoying. I think it's a good time to start already.
There was a problem hiding this comment.
Can we instrument the :json-rpc/call effect instead?
We can instrument the function call, but in re-frame, events just return data, so we need to instrument the event handlers themselves because the status-im.common.json-rpc.events/call is never called directly.
ilmotta
force-pushed
the
ilmotta.18126/relevant-tokens
branch
from
59f1ab8 to
456e8ef
Compare
yqrashawn
reviewed
Jan 29, 2024
src/status_im/contexts/communities/actions/addresses_for_permissions/events.cljs
Outdated
Show resolved
Hide resolved
ilmotta
force-pushed
the
ilmotta.18126/relevant-tokens
branch
2 times, most recently
from
df9f5e0 to
3ead902
Compare
Samyoul
added
the
community-account-selection
yqrashawn
approved these changes
Feb 6, 2024
ilmotta
force-pushed
the
ilmotta.18126/relevant-tokens
branch
2 times, most recently
from
5486008 to
5466e1f
Compare
ilmotta
commented
Feb 6, 2024
|
|
||
| (= type constants/community-token-type-erc721) | ||
| (assoc :collectible-name name | ||
| :collectible-img-src (resources/get-mock-image :collectible)) |
There was a problem hiding this comment.
FYI: I'll create a follow-up issue to use the real collectible image. It just didn't seem important for this PR since there's a bunch of things going on in the status-go side.
Use with-precision to show prettier amounts (workaround) For reference, see status-im/status-desktop#8640
ilmotta
force-pushed
the
ilmotta.18126/relevant-tokens
branch
from
5466e1f to
dae24ea
Compare
clauxx
approved these changes
Feb 8, 2024
19% of end-end tests have passedFailed tests (38)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (9)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #18126
Summary
Show relevant tokens in account selection step. Related status-go PR status-im/status-go#4631.
Things that surely can be done in follow-ups:
quo.wallet.account-permissionsto show collectibles in the token details section.Things that could become follow-up/improvement PRs:
Review notes
Areas that may be impacted
None because all affected code is behind a disabled feature flag.
Screenshot of the functionality
Steps to test
status-im.config/community-accounts-selection-enabled?adminpermissions / minting Owner token makes the community token gated for any user #18502 is solved, and to test this PR with minted tokens and collectibles, you will need to apply a workaround to forcefully go to the new join community flows.Workaround
status: ready